test/cli.test.js (1)

160-211: Add forwarding assertion here too for flow parity.

This test validates sandbox get and sandbox connect, but unlike the default-path test it does not assert forward start --background 18789 beta. Adding it will better guard reconnect regressions for explicit-name dispatch.

✅ Suggested assertion

     expect(r.code).toBe(0);
     expect(r.out.includes("Reconnecting to sandbox 'beta'")).toBeTruthy();
     const log = fs.readFileSync(markerFile, "utf8");
     expect(log).toContain("sandbox get beta");
+    expect(log).toContain("forward start --background 18789 beta");
     expect(log).toContain("sandbox connect beta");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/cli.test.js` around lines 160 - 211, The test "reconnect accepts an
explicit sandbox name" is missing the forwarding assertion present in the
default-path test; after reading the markerFile (variable markerFile) add an
assertion that the recorded command log includes the forward invocation for the
named sandbox — specifically assert markerFile contents contain "forward start
--background 18789 beta" (or at minimum that it contains "forward start" and
"beta" together) to ensure the openshell script received the forwarding command
when runWithEnv("reconnect beta") triggers sandbox connect; place this check
alongside the existing expectations that assert "sandbox get beta" and "sandbox
connect beta".

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/cli.test.js (2)

152-157: Consider asserting command order, not only presence.

These checks currently use toContain, which won’t catch sequence regressions (e.g., connect before forward start). Since reconnect orchestration order matters, index-based order assertions would make these tests more robust.

✅ Example order assertion pattern

     const log = fs.readFileSync(markerFile, "utf8");
-    expect(log).toContain("sandbox get alpha");
-    expect(log).toContain("forward start --background 18789 alpha");
-    expect(log).toContain("sandbox connect alpha");
+    const iGet = log.indexOf("sandbox get alpha");
+    const iForward = log.indexOf("forward start --background 18789 alpha");
+    const iConnect = log.indexOf("sandbox connect alpha");
+    expect(iGet).toBeGreaterThanOrEqual(0);
+    expect(iForward).toBeGreaterThan(iGet);
+    expect(iConnect).toBeGreaterThan(iForward);

Apply the same pattern to the named and missing-gateway reconnect tests (including gateway select before gateway start).

Also applies to: 206-211, 299-304

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/cli.test.js` around lines 152 - 157, Replace the loose toContain
assertions on markerFile/log in the reconnect tests with explicit order checks
using indexOf: compute indices for the expected strings (e.g., "sandbox get
alpha", "forward start --background 18789 alpha", "sandbox connect alpha") and
assert indexOf(first) !== -1 and indexOf(first) < indexOf(second) <
indexOf(third); do the same in the named and missing-gateway reconnect tests to
assert "gateway select" appears before "gateway start" (and any other ordered
steps), referencing the markerFile/log and the specific strings used in the
existing expectations.

---

169-180: Strengthen explicit-name test to catch fallback regressions.

Line 179 currently sets defaultSandbox to "beta", the same value passed in Line 201. If reconnect accidentally ignores explicit args and always uses default, this test would still pass.

🧪 Proposed test hardening

       JSON.stringify({
         sandboxes: {
+          alpha: {
+            name: "alpha",
+            model: "test-model",
+            provider: "nvidia-prod",
+            gpuEnabled: false,
+            policies: [],
+          },
           beta: {
             name: "beta",
             model: "test-model",
             provider: "nvidia-prod",
             gpuEnabled: false,
             policies: [],
           },
         },
-        defaultSandbox: "beta",
+        defaultSandbox: "alpha",
       }),

Also applies to: 201-211

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/cli.test.js` around lines 169 - 180, The test currently uses the same
value for defaultSandbox and the explicit sandbox arg, so update the fixture(s)
used in the "explicit-name" test to use a different defaultSandbox value (e.g.,
set defaultSandbox to "alpha" while leaving the explicit arg/sandbox name as
"beta") to ensure reconnect honors the explicit argument; update both the JSON
fixture at the sandboxes block (the object containing defaultSandbox) and the
similar fixture used around lines 201-211 so default and explicit names differ,
preserving existing names like "beta" for the explicit input.

test/cli.test.js (1)

106-323: Consider extracting a shared reconnect test fixture helper.

The repeated temp-home/registry/stub setup is sizable; a small helper would reduce duplication and make future reconnect scenarios easier to add.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/cli.test.js` around lines 106 - 323, Extract the repeated
temp-home/registry/stub setup in the three reconnect tests into a helper (e.g.,
setupReconnectFixture) that creates HOME, localBin, registryDir, markerFile (and
optionally stateFile), writes sandboxes.json and creates the openshell stub;
have it accept parameters for sandbox list, defaultSandbox, openshell behavior,
and initial state so tests can call setupReconnectFixture(...) and then call
runWithEnv("reconnect ..." , { HOME, PATH: `${localBin}:${process.env.PATH ||
""}` }, timeout). Update the tests to use this helper and assert using the
returned markerFile and paths instead of duplicating
fs.mkdirSync/fs.writeFileSync code; keep runWithEnv and the existing expectation
checks unchanged.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes